Skip to content

Make merchant accessible in PlayerPurchaseEvent #13062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make merchant accessible in PlayerPurchaseEvent #13062

wants to merge 1 commit into from

Conversation

@NoyException
Copy link

When using a custom merchant, it's common to want to know whether a recipe is just used by listening PlayerPurchaseEvent. However, if we couldn't know which merchant players are trading with, we can't reach that goal.

So I just simply add the approach.

@NoyException NoyException requested a review from a team as a code owner September 7, 2025 16:43
@github-project-automation github-project-automation bot moved this to Awaiting review in Paper PR Queue Sep 7, 2025
lynxplay
lynxplay reviewed Sep 7, 2025
View reviewed changes
Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds sensible, however now we are kinda doubling the #getVillager method in the PlayerTradeEvent as #getMerchant yields the same.

It might be beneficial to mark the #getVillager method as obsolete, overide the #getMerchant method to yield an AbstractVillager in PlayerTradeEvent and then have #getVillager link to it.

@NoyException
Copy link
Author

Sounds sensible, however now we are kinda doubling the #getVillager method in the PlayerTradeEvent as #getMerchant yields the same.

It might be beneficial to mark the #getVillager method as obsolete, overide the #getMerchant method to yield an AbstractVillager in PlayerTradeEvent and then have #getVillager link to it.

You're absolutely right! I have made the changes as you said.

lynxplay
lynxplay requested changes Sep 9, 2025
View reviewed changes
Copy link
Contributor

@lynxplay lynxplay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither of the events should be using jetbrains null-annotations, they are using jspecify already so should be fine without NotNull

@github-project-automation github-project-automation bot moved this from Awaiting review to Changes required in Paper PR Queue Sep 9, 2025
@NoyException
Copy link
Author

Neither of the events should be using jetbrains null-annotations, they are using jspecify already so should be fine without NotNull

Sure! I've corrected those. Could you please review again?

Owen1212055
Owen1212055 approved these changes Oct 17, 2025
View reviewed changes
Copy link
Member

@Owen1212055 Owen1212055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont really agree with obsoleting getVillager, especially because trade event can only be a villager. Otherwise looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Owen1212055 Owen1212055 Owen1212055 approved these changes

@lynxplay lynxplay Awaiting requested review from lynxplay

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

Paper PR Queue
Status: Changes required

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@NoyException @Owen1212055 @lynxplay